-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
client, server: implement configurable wire message size limits. #172
base: main
Are you sure you want to change the base?
client, server: implement configurable wire message size limits. #172
Conversation
a59fcd6
to
454025b
Compare
454025b
to
fbe97d4
Compare
I wonder if this is necessary. The protocol allows for 16 MB but then we hardcode in the limit of 4 MB. I wonder if we can limit this change to make it configurable but keep the protocol max at 16 MB. We also don't seem to properly implement what the current language is, if the first byte is non-zero we should immediately fail with a protocol error rather than attempting to discard more than what the protocol allows. While I'm sure there are use cases where more than 16 MB could be used, I'm not sure about just making this almost boundless on an object that is expected to be held and parsed in memory. Its better for the rpcs on top to implement their own streaming or framing. Related to server/client agreeing. It is possible that we would want a client that maintains a 4 MB send since it may not know whether the server supports more, although allowing a larger response. Is that currently possible here? Could also be useful to allow getting the behavior it is now where the client does not even enforce the limit but just allows the server to fail. In that case it would be good to preserve the "max" in the error from the server. |
Good point. I can set the max. allowed message size to 16MB.
I can take a stab at it and see if I can rework the PR with these in mind. I think if we split the current max. allowed length to a max. allowed send and a max. allowed receive limit, plus update both the client and server side options for setting the send limit to interpret 0 as "no limit/don't check", then we could do all of the above. And we could probably also then restore the default behavior (no client or server options used) to the original/current one. |
6e89eda
to
8c0eeb3
Compare
@dmcgowan Here is my first stab at addressing the review comments: This now
What is still missing are
|
e01a569
to
9e19b2d
Compare
@dmcgowan Updated to
I'm not sure if disabled sender-side size check is still the default behavior we want, or if we'd rather just want a programmatic way to explicitly restore the old behavior. If it is the latter, I think we could do that by providing extra |
Implement configurable limits for the maximum accepted message size of the wire protocol. The default limit can be overridden using the WithClientWireMessageLimit() option for clients and using the WithServerWireMessageLimit() option for servers. Add exported constants for the minimum, maximum and default limits. Signed-off-by: Krisztian Litkey <[email protected]>
Adjust unit test to accomodate for altered internal interfaces. Add unit tests to exercise the new message size limit options. Signed-off-by: Krisztian Litkey <[email protected]>
9e19b2d
to
7a70e2e
Compare
This PR implements configurable limits for the maximum accepted message size of the wire protocol. The default limits can be overridden using the new
WithClientWireMessageLimit()
option for clients andWithServerWireMessageLimit()
option for servers.In particular this PR includes commits to
@dmcgowan These options accept a maximum limit of 2^31-1 and a minimum limit of 4K. Therefore this PR effectively changes the protocol (as reflected in the updated protocol description) to use a reserved most significant bit in the frame data length field instead of a reserved most significant byte. Is this fine ? Or should we leave a few more bits reserved for future use, for instance by limiting the max. wire message size to 256M-1 leaving 4 bits unused ?
Updates: